Skip to content

Add changes to get custom section data from xrt::elf#9664

Open
rbramand-xilinx wants to merge 2 commits intoXilinx:masterfrom
rbramand-xilinx:custom_section
Open

Add changes to get custom section data from xrt::elf#9664
rbramand-xilinx wants to merge 2 commits intoXilinx:masterfrom
rbramand-xilinx:custom_section

Conversation

@rbramand-xilinx
Copy link
Collaborator

Problem solved by the commit

Added changes as per spec - https://amd.atlassian.net/wiki/spaces/AIE/pages/1341720313/Spec+of+xrt+elf+APIs to get custom section data from xrt::elf object.

Custom section can be global level (not associated with any kernel/compute unit), kernel level and instance level. Added apis in respective classes to get the data.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

It is a new feature

How problem was solved, alternative solutions (if any) and why they were rejected

Added public API get_custom_section in xrt::elf class to get global custom section data, in xrt::elf::kernel class to get kernel custom section data and in xrt::elf::kernel::instance class to get instance custom section data

The return type for all these API's is xrt::detail::span. Ideally it should be std::span but as XRT uses c++17 and std::span is available from c++20, we are using a lightweight span class implemented in XRT. Move to std::span when XRT moves to c++ 20

Risks (if any) associated the changes in the commit

Low

What has been tested and how, request additional testing if necessary

Tested using full ELF with custom sections and retrieving both text and binary custom section data works as expected

Documentation impact (if any)

Added doxygen comments wherever needed.

Example showing how to retrieve custom section data :
1. Global-level custom section
Custom sections that are not tied to any kernel or instance are read from the xrt::elf object:

#include "xrt/experimental/xrt_elf.h"
#include <fstream>

xrt::elf elf("test.elf");

// Read global custom section with section name "Global_config"
auto all_data = elf.get_custom_section("Global_config");

// Use the data (e.g. write to file)
std::ofstream out("global_config.bin", std::ios::binary);
out.write(all_data.data(), static_cast<std::streamsize>(all_data.size()));

2. Kernel-level custom section
Sections attached to a kernel (not to a specific instance) are read from the corresponding xrt::elf::kernel:

xrt::elf elf("test.elf");
auto kernels = elf.get_kernels();

// read custom section from kernel **DPU** 
for (const auto& kernel : kernels) {
  if (kernel.get_name() != "DPU")
    continue;

  // Read kernel-level custom section (e.g. "METADATA", "CONFIG")
  auto metadata = kernel.get_custom_section("METADATA");

  std::string out_name = std::string("DPU_") + "METADATA.bin";
  std::ofstream out(out_name, std::ios::binary);
  out.write(metadata.data(), static_cast<std::streamsize>(metadata.size()));
  break;
}

3. Instance-level custom section
Sections attached to a specific kernel instance are read from the corresponding xrt::elf::kernel::instance:

xrt::elf elf("my_application.elf");
auto kernels = elf.get_kernels();

for (const auto& kernel : kernels) {
  if (kernel.get_name() != "RESNET")
    continue;

  for (const auto& instance : kernel.get_instances()) {
    // Read instance-level custom section
    auto config = instance.get_custom_section("CONFIG");

    // e.g. filename: kernel_name + "_" + instance_name + "_config"
    std::string out_name = kernel.get_name() + "_" + instance.get_name() + "_config.bin";
    std::ofstream out(out_name, std::ios::binary);
    out.write(config.data(), static_cast<std::streamsize>(config.size()));
  }
  break;
}

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds public APIs to retrieve custom ELF section data from xrt::elf at global, kernel, and kernel-instance scope, and wires up parsing/storage of those custom sections in the ELF runtime implementation.

Changes:

  • Added get_custom_section() APIs on xrt::elf, xrt::elf::kernel, and xrt::elf::kernel::instance returning xrt::detail::span<const char>.
  • Implemented parsing of custom sections (SHT_LOUSER + 1) and routing of section data into global/kernel/instance maps.
  • Refactored ELF parsing entry point from .group-only parsing to a unified parse_sections() flow.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File Description
src/runtime_src/core/include/xrt/experimental/xrt_elf.h Exposes new public APIs (and doxygen) for retrieving custom section data at multiple scopes.
src/runtime_src/core/common/api/xrt_elf.cpp Implements storage and lookup for custom sections, plus parsing changes to collect and classify custom sections.
src/runtime_src/core/common/api/elf_int.h Adds internal storage for global custom sections and declares new parsing helpers / accessor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +328 to +330
// Map for custom sections of an instance
// key - custom section name, value - custom section data
std::map<std::string, detail::span<const char>> m_custom_section_map;
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instance_impl also stores custom section data as detail::span<const char>, which is a non-owning view into elf_impl::m_elfio section memory. Since xrt::elf::kernel::instance objects can be copied out and outlive the parent xrt::elf, instance::get_custom_section() can return a dangling span after the xrt::elf is destroyed. Please change storage to an owning representation (or a safe indirection back to elf_impl with lifetime checks) so the returned span never references freed memory.

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +93
/**
* get_custom_section() - Get custom section data of an instance from ELF
*
* @param section_name
* Name of the custom section
*
* @return
* A span representing the custom section data
*
* @note
* Returns xrt::detail::span (lightweight span) for now. Will switch to
* std::span when XRT uses C++20 by default.
*/
XRT_API_EXPORT
xrt::detail::span<const char>
get_custom_section(const std::string& section_name) const;
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel::instance::get_custom_section() returns a non-owning span, but the comment doesn’t state how long the returned view remains valid (what object owns the backing bytes). Please document the lifetime/ownership guarantees explicitly to prevent callers from using a span after the backing ELF storage has been released.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid point. At least state that the span is valid only as long as the xrt::elf is in scope.

Comment on lines +152 to +167
/**
* get_custom_section() - Get custom section data of a kernel from ELF
*
* @param section_name
* Name of the custom section
*
* @return
* A span representing the custom section data
*
* @note
* Returns xrt::detail::span (lightweight span) for now. Will switch to
* std::span when XRT uses C++20 by default.
*/
XRT_API_EXPORT
xrt::detail::span<const char>
get_custom_section(const std::string& section_name) const;
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel::get_custom_section() returns a non-owning span but the doxygen does not describe lifetime/ownership of the returned data. Please add an explicit lifetime note (e.g., valid while the originating ELF object and its backing storage remain alive) so callers know whether they must copy the data before storing it.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, yes.

Comment on lines +247 to +250
// Map for custom sections of a kernel
// key - custom section name, value - custom section data
std::map<std::string, detail::span<const char>> m_custom_section_map;

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel_impl stores custom section data as detail::span<const char> in m_custom_section_map. The span points into ELFIO::section data owned by elf_impl::m_elfio, but xrt::elf::kernel objects can outlive the parent xrt::elf (they hold independent shared_ptr<kernel_impl>). This makes kernel::get_custom_section() able to return a dangling view (use-after-free) once the parent xrt::elf is destroyed. Consider storing an owning buffer for each custom section (e.g., std::vector<char>/shared buffer) or redesigning so kernel_impl/instance_impl retain safe access to the backing ELF data without creating ownership cycles (e.g., keep indices + weak_ptr to elf_impl and fail if expired).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stsoe, after checking this I am thinking should we store custom section data for kernel and instance in vector and return span of this? or just documenting about the lifetime of data is sufficient. Please suggest. Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't xrt::elf own the ELFIO? It's fair enough that xrt::elf can return views to its own managed data that is valid only as long as the xrt::elf object is alive. Alternatively, I guess you could make all the embedded impl classes hold a reference to the parent impl. Not sure I would worry about this other than documenting. Good point though.

Signed-off-by: rahul <[email protected]>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

get_kernel_subkernel_from_symtab(symtab_idx);

if (!add_custom_section_to_kernel_or_instance(
kernel_name, subkernel_name, sec_name, data))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [hicpp-braces-around-statements]

Suggested change
kernel_name, subkernel_name, sec_name, data))
kernel_name, subkernel_name, sec_name, data)) {

src/runtime_src/core/common/api/xrt_elf.cpp:626:

-           "' which is not in the ELF");
+           "' which is not in the ELF");
+ }

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is elf_impl is elf_inf.h ? This is contrary to all other XRT first class objects where impl is in xrt_<fco>.cpp?

Copy link
Collaborator

@larry9523 larry9523 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's refactor this with a simpler solution, removing the complicity of global/kernel/instance level. We only support one level of customized section. AIEBU and XRT only cares the name/value pair.

@rbramand-xilinx
Copy link
Collaborator Author

Why is elf_impl is elf_inf.h ? This is contrary to all other XRT first class objects where impl is in xrt_<fco>.cpp?

Hi Soren, from xrt_module.cpp we call multiple ELF functions for data that is parsed and stored in elf_impl object. We can expose this data using elf_int functions like other xrt objects but then there would be multiple such functions so thought of putting elf_impl implementation in elf_int.h so we can directly call functions using elf_impl objects instead of elf_int functions.
As elf_int.h is internal it wont cause any ABI breakage is what I thought.
Let me know if we need to refactor this and move it to xrt_elf.cpp and use elf_int functions for data. Thanks

@rbramand-xilinx
Copy link
Collaborator Author

Let's refactor this with a simpler solution, removing the complicity of global/kernel/instance level. We only support one level of customized section. AIEBU and XRT only cares the name/value pair.

Sure @larry9523 , I will make changes accordingly. Should we update spec- https://amd.atlassian.net/wiki/spaces/AIE/pages/1341720313/Spec+of+xrt+elf+APIs to reflect same ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants